-
Notifications
You must be signed in to change notification settings - Fork 237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CP-44372: Integrate NRPE UI with backend interface #3226
CP-44372: Integrate NRPE UI with backend interface #3226
Conversation
6ac41a2
to
53fdfa0
Compare
53fdfa0
to
3928300
Compare
3928300
to
177090a
Compare
@BengangY I have made some corrections on a branch in my fork, could you please enable PRs on your fork so I can raise a PR to it. Alternatively you can just pull the top 3 commits from here https://github.com/kc284/xenadmin/commits/private/bengangy/CP-44371, but it is better if I could send a PR with comments to explain the changes. This is one part of corrections, there are some more changes needed, but I didn't make them as they require greater code shifting so I'll leave it to you. Please see my other comments in this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above.
A general remark: normally for new controls we would add localized resx placeholders, but at the moment we have stopped shipping localized resources on master, so the palceholders are not needed. This is the first PR with missing placeholders, so please ensure it is merged after #3238 which disables the unit tests. |
Signed-off-by: Konstantina Chremmou <[email protected]>
Signed-off-by: Konstantina Chremmou <[email protected]>
Signed-off-by: Konstantina Chremmou <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't done a complete review but noticed a couple of things while skim-reading
1a9de68
to
6c81585
Compare
dbf168c
to
96175e1
Compare
96175e1
to
c06defe
Compare
@@ -317,6 +318,12 @@ private void Build() | |||
dialog.ShowDialog(Program.MainWindow); | |||
} | |||
} | |||
if (isPoolOrStandalone && Helpers.XapiEqualOrGreater_23_27_0(connection) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is isPoolOrStandalone
correct here? This means that if someone wants to set different settings for only one host in a multi-host pool, they cannot do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's correct. Have confirmed with Enzo that NRPE page will be shown for a pool and standalone host, not shown for a host in a multi-host pool.
|
Updated. Now it will check when the datagridview lost focus each time.
Updated. Added a reselection process when error occurs. Code likes:
It will select the first row and then select the error row. The reason is that if it only selects the error row, if the user already selected the error row and scrolled down the datagrid and click to a different tab, then click the OK button, it will not focus on the error row. So it needs a switch process. |
|
||
public AsyncAction SaveSettings() | ||
{ | ||
return new NRPEUpdateAction(_clone, _nrpeCurrentConfig, _nrpeOriginalConfig, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't spot this earlier: why are we suppressing the history? This means that there is not record of the action on the events TabPage. Same for line 195 and NRPERetrieveAction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, actually we do suppress this for more properties that we save. It probably needs revisiting at a higher level, so it is probably a no-action for this PR.
One more question: I noticed that when we disable NRPE and then we renable it, the previous settings are reset and the user has to set up thresholds again. Is this by design? (Asking because sometimes features do remember the settings when disabled). |
By design, all the settings displayed in NRPE UI are retrieved from the host's configuration file when opening the dialog. If customers check 'enabled NRPE' and customize the setting, clicking 'OK' will rewrite the configuration file for each host, which means the previous settings won't be reset by following single-op like disable and re-enable. But there's actually one special case we can discuss, if customers modify thresholds together with checking 'disable NRPE', and then click the 'OK' button, the host will only disable the NRPE without rewriting the configuration file, so next time when enabling NRPE again, those changes won't be seen from UI. |
Added below use case on XenCenter
Enable/Disable NRPE for host
Configure allow_hosts /Enable Debug/Enable SSL_logging
Set threshold for each check
View existing settings for each host
Validation for the input
Same function for the pool